Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/8.0] Support assignment to multiple refs in trim analyzer #90936

Merged
merged 4 commits into from
Aug 23, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 22, 2023

Backport of #90287 to release/8.0

/cc @sbomer

Customer Impact

Analyzer would throw an exception, producing the following error with certain specific code patterns involving assignments to multiple refs:

'ILLink.RoslynAnalyzer.DynamicallyAccessedMembersAnalyzer' threw an exception of type 'System.InvalidOperationException' with message 'Operation is not valid due to the current state of the object.'.

For example, the following code would produce this error:

(b ? ref f1 : ref f2) = v;

Testing

Added testcases to validate that this and similar patterns no longer throw and produce reasonable analysis results (even though it is unlikely that such a pattern will be used in a way that is meaningful for the purpose of trim warnings).

Risk

Low risk. This fixes an analyzer crash without changing the analysis results for existing code. There's a fair amount of code in this change, but any risk is limited to the analyzer that runs during build, not directly affecting the runtime behavior or the output binary.

@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Aug 22, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Aug 22, 2023
@ghost
Copy link

ghost commented Aug 22, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #90287 to release/8.0

/cc @sbomer

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

linkable-framework, area-Tools-ILLink

Milestone: -

@sbomer sbomer requested review from agocke and vitek-karas August 22, 2023 17:24
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved. once you have a code review and a green ci you can merge.

@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Aug 22, 2023
@carlossanlop
Copy link
Member

@sbomer I can merge it for you, just ping me. No need to click on "Update branch" (it will restart the CI).

There is a CI failure in browser-wasm saying there's not enough disk space, so I assume it's unrelated. I re-ran the failed legs just in case, but they were showing up as warnings so I might just merge if they happen again.

@carlossanlop carlossanlop merged commit ac16ae1 into release/8.0 Aug 23, 2023
@carlossanlop carlossanlop deleted the backport/pr-90287-to-release/8.0 branch August 23, 2023 21:53
@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants